Skip to content

Comments

feat: add runtime hardening#57

Merged
Koopa0 merged 1 commit intomainfrom
feat/seacure-handling
Feb 15, 2026
Merged

feat: add runtime hardening#57
Koopa0 merged 1 commit intomainfrom
feat/seacure-handling

Conversation

@Koopa0
Copy link
Owner

@Koopa0 Koopa0 commented Feb 15, 2026

  • Add http.MaxBytesReader (1MB) and content length cap (32KB) to API chat handlers
  • Add ErrPathDenied sentinel and deniedPrefixes to security.Path for blocking prompt directory access
  • Add MaxKnowledgeContentSize (50KB) validation to knowledge_store tool
  • Update NewPath signature across all callers to support denied prefixes
  • Add unit tests for all new validation paths

  - Add http.MaxBytesReader (1MB) and content length cap (32KB) to API
    chat handlers
  - Add ErrPathDenied sentinel and deniedPrefixes to security.Path for
    blocking prompt directory access
  - Add MaxKnowledgeContentSize (50KB) validation to knowledge_store
    tool
  - Update NewPath signature across all callers to support denied
    prefixes
  - Add unit tests for all new validation paths
@Koopa0 Koopa0 merged commit 4630dca into main Feb 15, 2026
5 of 7 checks passed
@Koopa0 Koopa0 deleted the feat/seacure-handling branch February 15, 2026 08:11
Comment on lines +199 to +221
func TestChatSend_BodyTooLarge(t *testing.T) {
// Create a valid JSON body larger than maxRequestBodySize (1 MB).
// The content field must be large enough so the whole JSON exceeds the limit.
largeContent := strings.Repeat("x", maxRequestBodySize)
body, _ := json.Marshal(map[string]string{
"content": largeContent,
"sessionId": uuid.New().String(),
})

w := httptest.NewRecorder()
r := httptest.NewRequest(http.MethodPost, "/api/v1/chat", bytes.NewReader(body))

newTestChatHandler().send(w, r)

if w.Code != http.StatusRequestEntityTooLarge {
t.Fatalf("send(>1MB body) status = %d, want %d\nbody: %s", w.Code, http.StatusRequestEntityTooLarge, w.Body.String())
}

errResp := decodeErrorEnvelope(t, w)
if errResp.Code != "body_too_large" {
t.Errorf("send(>1MB body) code = %q, want %q", errResp.Code, "body_too_large")
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security/Robustness:
The test TestChatSend_BodyTooLarge verifies the status code and error code, but does not assert that the response body is free from sensitive or unexpected information. To strengthen the test, add assertions to ensure the response body does not contain any partial or leaked data.

Recommended solution:

if strings.Contains(w.Body.String(), "hello") {
    t.Error("send(>1MB body) response should not contain partial content")
}

Comment on lines +89 to 90
handler = corsMiddleware(cfg.CORSOrigins)(handler)
handler = loggingMiddleware(logger)(handler)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Middleware Order: CORS and Logging

The CORS middleware is applied before the logging middleware. This means that CORS preflight requests (OPTIONS) may not be logged, potentially omitting important request traces for debugging or auditing. Consider placing the logging middleware as the outermost layer (before CORS) to ensure all requests, including preflight, are logged:

handler = loggingMiddleware(logger)(handler)
handler = corsMiddleware(cfg.CORSOrigins)(handler)

This change will improve observability without affecting CORS behavior.

Comment on lines +130 to 131
if subtle.ConstantTimeCompare(actualSig, expectedSig) != 1 {
return ErrCSRFInvalid

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential ambiguity in signature length handling:

The comparison subtle.ConstantTimeCompare(actualSig, expectedSig) will return 0 if the lengths differ, but this is implicit. For clarity and maintainability, consider explicitly checking that len(actualSig) == len(expectedSig) before performing the comparison. This makes the logic clearer and avoids confusion for future maintainers.

Recommended solution:

if len(actualSig) != len(expectedSig) || subtle.ConstantTimeCompare(actualSig, expectedSig) != 1 {
    return ErrCSRFInvalid
}

Comment on lines +191 to 192
if subtle.ConstantTimeCompare(actualSig, expectedSig) != 1 {
return ErrCSRFInvalid

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential ambiguity in signature length handling:

As in the user-bound CSRF check, subtle.ConstantTimeCompare(actualSig, expectedSig) will return 0 if the lengths differ, but this is implicit. For clarity and maintainability, explicitly check len(actualSig) == len(expectedSig) before comparison.

Recommended solution:

if len(actualSig) != len(expectedSig) || subtle.ConstantTimeCompare(actualSig, expectedSig) != 1 {
    return ErrCSRFInvalid
}

Comment on lines +115 to 118
pathValidator, err := security.NewPath([]string{os.TempDir()}, nil)
if err != nil {
t.Fatalf("creating path validator: %v", err)
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error Handling and Test Robustness:
The error handling for security.NewPath is abrupt and does not provide actionable context if the path validator fails. If the failure is due to environmental issues (e.g., os.TempDir() is not writable or accessible), the test will simply fail without guidance. Consider enhancing the error message to include the value of os.TempDir() and possible remediation steps, such as checking directory permissions or environment configuration.

Recommended Solution:

if err != nil {
    t.Fatalf("creating path validator for temp dir '%s': %v. Ensure the directory is writable and accessible.", os.TempDir(), err)
}

Comment on lines 54 to 88
absAllowedDirs = append(absAllowedDirs, absDir)
}

// Convert denied prefixes to absolute paths
absDenied := make([]string, 0, len(deniedPrefixes))
for _, prefix := range deniedPrefixes {
absPrefix, err := filepath.Abs(prefix)
if err != nil {
return nil, fmt.Errorf("unable to resolve denied prefix %s: %w", prefix, err)
}
absDenied = append(absDenied, absPrefix)
}

return &Path{
allowedDirs: absAllowedDirs,
workDir: workDir,
allowedDirs: absAllowedDirs,
deniedPrefixes: absDenied,
workDir: workDir,
}, nil
}

// isPathDenied checks if a path matches any denied prefix.
// Uses case-insensitive comparison to handle case-insensitive filesystems (macOS HFS+).
func (v *Path) isPathDenied(absPath string) bool {
for _, denied := range v.deniedPrefixes {
deniedWithSep := filepath.Clean(denied) + string(filepath.Separator)
if strings.EqualFold(absPath, denied) || strings.HasPrefix(strings.ToLower(absPath+string(filepath.Separator)), strings.ToLower(deniedWithSep)) {
return true
}
}
return false
}

// isPathInAllowedDirs checks if a path is within allowed directories
// Returns true if path is in working directory or any allowed directory
func (v *Path) isPathInAllowedDirs(absPath string) bool {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential directory containment flaw in isPathInAllowedDirs

The use of strings.HasPrefix to check if a path is within an allowed directory can be unsafe if directory names are prefixes of others (e.g., /tmp/foo and /tmp/foobar). This could allow unintended access:

if strings.HasPrefix(absPathWithSep, dirNorm) || absPath == dir {
    return true
}

Recommendation: Use filepath.Rel(dir, absPath) and check that the result does not start with .. or is not equal to ... This provides a more robust containment check and prevents directory traversal attacks.

Comment on lines 156 to 169
return "", fmt.Errorf("%w: access denied", ErrPathOutsideAllowed)
}

// 3b. Check if path matches a denied prefix (e.g. prompts/)
if v.isPathDenied(absPath) {
slog.Warn("path denied by prefix rule",
"path", absPath,
"security_event", "denied_prefix_access_attempt")
return "", fmt.Errorf("%w: access denied", ErrPathDenied)
}

// 4. Resolve symbolic links (prevent bypassing restrictions through symlinks)
realPath, err := filepath.EvalSymlinks(absPath)
if err != nil {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Denied prefix check missing after symlink resolution

After resolving symlinks with filepath.EvalSymlinks, the code checks if the resolved path is within allowed directories, but does not re-check denied prefixes. This could allow a symlink to bypass denied prefix restrictions:

if realPath != absPath {
    if !v.isPathInAllowedDirs(realPath) {
        // ...
        return "", fmt.Errorf("%w: access denied", ErrSymlinkOutsideAllowed)
    }
    absPath = realPath
}

Recommendation: After symlink resolution, also check isPathDenied(realPath) and return an error if the resolved path matches a denied prefix.

//
// // Create tools with security validators
// pathVal, _ := security.NewPath([]string{"/allowed/path"})
// pathVal, _ := security.NewPath([]string{"/allowed/path"}, nil)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential Security and Logic Issue: Error from security.NewPath is Ignored

The example uses a blank identifier for the error returned by security.NewPath:

pathVal, _ := security.NewPath([]string{"/allowed/path"}, nil)

If security.NewPath fails (e.g., due to invalid input), this error will be silently ignored, potentially resulting in a nil or insecure path validator. This could compromise the security guarantees of the file tools.

Recommended Solution:
Always check and handle the error returned by security.NewPath:

pathVal, err := security.NewPath([]string{"/allowed/path"}, nil)
if err != nil {
    return err
}

This ensures that the path validator is correctly initialized and that any initialization errors are surfaced early.

Comment on lines 8 to 14

func TestFile_Constructor(t *testing.T) {
t.Run("valid inputs", func(t *testing.T) {
pathVal, err := security.NewPath([]string{"/tmp"})
pathVal, err := security.NewPath([]string{"/tmp"}, nil)
if err != nil {
t.Fatalf("creating path validator: %v", err)
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Insufficient validation of constructed object:
The test only checks that ft is non-nil and that no error occurred, but does not verify the properties or state of the constructed File object. This could allow subtle initialization bugs to go undetected.

Recommendation:
Add assertions to verify the expected properties and state of the File object after construction, for example:

if ft.PathValidator != pathVal {
    t.Errorf("PathValidator not set correctly")
}

Comment on lines 97 to 98
if err != nil {
t.Skip("could not create validator")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipping the test when the validator cannot be created (t.Skip("could not create validator")) may mask persistent setup or configuration issues, resulting in incomplete test coverage. Instead, consider failing the test with a clear error message to ensure that validator initialization problems are detected early:

t.Fatalf("could not create validator: %v", err)

This approach provides immediate feedback and aids in debugging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant